-
Notifications
You must be signed in to change notification settings - Fork 356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: experiment list batch [DET-3001] #866
feat: experiment list batch [DET-3001] #866
Conversation
b1a9d4f
to
02d6088
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work here it looks great and we get an animation going on! I added some comments below
- kill triggered errors (stale state): I ran into a situation where the react view has stale state (because of polling) then trying to kill resulted in getting an error from the server. I'm not sure what we can do here with lack of streaming. I know with the new API I'm making these post endpoints idempotent thus it won't return an error and this would be a non issue.
- I could see people miss clicking the area around the row checkbox: could we have bigger input select area. or deactivate click handler for it? this was something we explicitly added to our Elm table. could be for future work but before we release the table?
I think we can't fulfill
For
View in TensorBoard
- If ANY of the selected experiment can not be viewed in TensorBoard, disable the batchView in TensorBoard
operation.
with the information we currently get on this page.
Oops I had logic it always force it to be true while testing and forgot to remove it, thanks for catching this! |
e27782e
to
b0ccc27
Compare
b0ccc27
to
e70a404
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Could we keep the selection after the batch actions are settled? I think we had some discussions on this previously (for Elm). This would also be keeping the existing behavior.
e70a404
to
964df34
Compare
Description
Test Plan
Commentary (optional)